From a3f51001c03a2ac3ac6562b363cd50a4fa3f1b63 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sat, 13 Jan 2018 19:57:51 -0800 Subject: [PATCH] rdbms: clean up non-native Database::replace() code * Make sure all unique keys specified have all their values provided to avoid large bogus DELETEs. Do not ignore them in such cases either, as that would cause inconsistencies between the native and non-native case. Use an exception. * Make ChangeTags caller clearer that the list of indexes is not a list of fields for a single index. Also, avoid mentioning indexes for values not defined in the new records, as this causes errors or inconsistencies with the native vs non-native case. * This also fixes the "Undefined index: ts_log_id" error when running unit tests on postgres. Change-Id: I30263df22066bd6d4836202b1bcad5d1aa1e7383 --- includes/changetags/ChangeTags.php | 19 ++++--- includes/libs/rdbms/database/Database.php | 56 +++++++++---------- .../libs/rdbms/database/DatabaseSQLTest.php | 12 ++-- 3 files changed, 43 insertions(+), 44 deletions(-) diff --git a/includes/changetags/ChangeTags.php b/includes/changetags/ChangeTags.php index 95848ea085..7e4dd006ac 100644 --- a/includes/changetags/ChangeTags.php +++ b/includes/changetags/ChangeTags.php @@ -408,19 +408,24 @@ class ChangeTags { sort( $prevTags ); sort( $newTags ); if ( $prevTags == $newTags ) { - // No change. return false; } if ( !$newTags ) { - // no tags left, so delete the row altogether + // No tags left, so delete the row altogether $dbw->delete( 'tag_summary', $tsConds, __METHOD__ ); } else { - $dbw->replace( 'tag_summary', - [ 'ts_rev_id', 'ts_rc_id', 'ts_log_id' ], - array_filter( array_merge( $tsConds, [ 'ts_tags' => implode( ',', $newTags ) ] ) ), - __METHOD__ - ); + // Specify the non-DEFAULT value columns in the INSERT/REPLACE clause + $row = array_filter( [ 'ts_tags' => implode( ',', $newTags ) ] + $tsConds ); + // Check the unique keys for conflicts, ignoring any NULL *_id values + $uniqueKeys = []; + foreach ( [ 'ts_rev_id', 'ts_rc_id', 'ts_log_id' ] as $uniqueColumn ) { + if ( isset( $row[$uniqueColumn] ) ) { + $uniqueKeys[] = [ $uniqueColumn ]; + } + } + + $dbw->replace( 'tag_summary', $uniqueKeys, $row, __METHOD__ ); } return true; diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 1efa9a1f2e..6d1cff7d6a 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -2238,49 +2238,43 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } public function replace( $table, $uniqueIndexes, $rows, $fname = __METHOD__ ) { - $quotedTable = $this->tableName( $table ); - if ( count( $rows ) == 0 ) { return; } - # Single row case + // Single row case if ( !is_array( reset( $rows ) ) ) { $rows = [ $rows ]; } - // @FXIME: this is not atomic, but a trx would break affectedRows() + // @FIXME: this is not atomic, but a trx would break affectedRows() foreach ( $rows as $row ) { - # Delete rows which collide - if ( $uniqueIndexes ) { - $sql = "DELETE FROM $quotedTable WHERE "; - $first = true; - foreach ( $uniqueIndexes as $index ) { - if ( $first ) { - $first = false; - $sql .= '( '; - } else { - $sql .= ' ) OR ( '; - } - if ( is_array( $index ) ) { - $first2 = true; - foreach ( $index as $col ) { - if ( $first2 ) { - $first2 = false; - } else { - $sql .= ' AND '; - } - $sql .= $col . '=' . $this->addQuotes( $row[$col] ); - } - } else { - $sql .= $index . '=' . $this->addQuotes( $row[$index] ); - } + // Delete rows which collide with this one + $indexWhereClauses = []; + foreach ( $uniqueIndexes as $index ) { + $indexColumns = (array)$index; + $indexRowValues = array_intersect_key( $row, array_flip( $indexColumns ) ); + if ( count( $indexRowValues ) != count( $indexColumns ) ) { + throw new DBUnexpectedError( + $this, + 'New record does not provide all values for unique key (' . + implode( ', ', $indexColumns ) . ')' + ); + } elseif ( in_array( null, $indexRowValues, true ) ) { + throw new DBUnexpectedError( + $this, + 'New record has a null value for unique key (' . + implode( ', ', $indexColumns ) . ')' + ); } - $sql .= ' )'; - $this->query( $sql, $fname ); + $indexWhereClauses[] = $this->makeList( $indexRowValues, LIST_AND ); + } + + if ( $indexWhereClauses ) { + $this->delete( $table, $this->makeList( $indexWhereClauses, LIST_OR ), $fname ); } - # Now insert the row + // Now insert the row $this->insert( $table, $row, $fname ); } } diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php index 23f7865058..d8ebeff170 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php @@ -560,7 +560,7 @@ class DatabaseSQLTest extends PHPUnit_Framework_TestCase { 'rows' => [ 'field' => 'text', 'field2' => 'text2' ], ], "DELETE FROM replace_table " . - "WHERE ( field='text' ); " . + "WHERE (field = 'text'); " . "INSERT INTO replace_table " . "(field,field2) " . "VALUES ('text','text2')" @@ -576,7 +576,7 @@ class DatabaseSQLTest extends PHPUnit_Framework_TestCase { ], ], "DELETE FROM module_deps " . - "WHERE ( md_module='module' AND md_skin='skin' ); " . + "WHERE (md_module = 'module' AND md_skin = 'skin'); " . "INSERT INTO module_deps " . "(md_module,md_skin,md_deps) " . "VALUES ('module','skin','deps')" @@ -598,12 +598,12 @@ class DatabaseSQLTest extends PHPUnit_Framework_TestCase { ], ], "DELETE FROM module_deps " . - "WHERE ( md_module='module' AND md_skin='skin' ); " . + "WHERE (md_module = 'module' AND md_skin = 'skin'); " . "INSERT INTO module_deps " . "(md_module,md_skin,md_deps) " . "VALUES ('module','skin','deps'); " . "DELETE FROM module_deps " . - "WHERE ( md_module='module2' AND md_skin='skin2' ); " . + "WHERE (md_module = 'module2' AND md_skin = 'skin2'); " . "INSERT INTO module_deps " . "(md_module,md_skin,md_deps) " . "VALUES ('module2','skin2','deps2')" @@ -625,12 +625,12 @@ class DatabaseSQLTest extends PHPUnit_Framework_TestCase { ], ], "DELETE FROM module_deps " . - "WHERE ( md_module='module' ) OR ( md_skin='skin' ); " . + "WHERE (md_module = 'module') OR (md_skin = 'skin'); " . "INSERT INTO module_deps " . "(md_module,md_skin,md_deps) " . "VALUES ('module','skin','deps'); " . "DELETE FROM module_deps " . - "WHERE ( md_module='module2' ) OR ( md_skin='skin2' ); " . + "WHERE (md_module = 'module2') OR (md_skin = 'skin2'); " . "INSERT INTO module_deps " . "(md_module,md_skin,md_deps) " . "VALUES ('module2','skin2','deps2')" -- 2.20.1